Skip to content

Refactor code to choose aggregate, network interface and creating storage volume; Also, the corresponding UT changes - #89

Open
sandeeplocharla wants to merge 2 commits into
mainfrom
bugfix/CSTACKEX-238
Open

Refactor code to choose aggregate, network interface and creating storage volume; Also, the corresponding UT changes#89
sandeeplocharla wants to merge 2 commits into
mainfrom
bugfix/CSTACKEX-238

Conversation

@sandeeplocharla

@sandeeplocharla sandeeplocharla commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Description

This PR has changes to refactor choosing an aggregate and its corresponding network interface and also creation of storage volume. Also, with this, volume creation would be done at the end, avoiding volume creation cleanup if in case there's a chance of failure during aggregate or network interface selection.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • Build/CI
  • Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

Screenshot 2026-08-10 at 7 23 42 AM Screenshot 2026-08-10 at 7 24 42 AM Screenshot 2026-08-10 at 7 25 06 AM Screenshot 2026-08-10 at 7 27 30 AM [ChoosingAggregateRefactorLogs_iSCSi.rtf](https://github.com/user-attachments/files/30884333/ChoosingAggregateRefactorLogs_iSCSi.rtf) Screenshot 2026-08-10 at 7 33 52 AM Screenshot 2026-08-10 at 7 34 48 AM Screenshot 2026-08-10 at 7 35 01 AM Screenshot 2026-08-10 at 7 36 07 AM [choosingAggregateRefactorLogs_NFS3.rtf](https://github.com/user-attachments/files/30884337/choosingAggregateRefactorLogs_NFS3.rtf)

…rage volume; Also, the corresponding UT changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the ONTAP primary storage initialization flow to explicitly select an aggregate (and a node-affine data LIF) before creating the backing FlexVol, so volume creation happens last and can avoid cleanup work if earlier selection steps fail.

Changes:

  • Split aggregate selection into a dedicated chooseAggregate(size) method and pass the chosen aggregate into volume creation.
  • Update data LIF selection to require the chosen aggregate (for deterministic node affinity) and move LIF selection before volume creation in the datastore lifecycle.
  • Add stricter validation around aggregate/node presence for LIF affinity.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java Introduces explicit aggregate selection and requires aggregate input for LIF selection and volume creation.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java Reorders initialization to choose aggregate + LIF first, then create the FlexVol on the selected aggregate.
Suppressed comments (2)

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:610

  • getNetworkInterface wraps unexpected exceptions in a CloudRuntimeException but drops the original cause, which makes upstream error handling/debugging harder (especially since callers may rewrap again).
        } catch (CloudRuntimeException e) {
            throw e;
        } catch (Exception e) {
            logger.error("Exception while retrieving network interfaces: ", e);
            throw new CloudRuntimeException("Failed to retrieve network interfaces: " + e.getMessage());
        }

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:233

  • This refactor changes the public API by removing the previous overloads createStorageVolume(String, Long) and getNetworkInterface(); however, the repo still contains unit tests that call the old signatures (e.g. StorageStrategyTest and OntapPrimaryDatastoreLifecycleTest). As-is, this will fail compilation unless those tests are updated (or compatibility wrappers are added).
    public Aggregate chooseAggregate(Long size) {
        String svmName = storage.getSvmName();
        if (aggregates == null || aggregates.isEmpty()) {
            logger.error("No aggregates available to create volume on SVM " + svmName);
            throw new CloudRuntimeException("No aggregates available to create volume on SVM " + svmName);
        }
        if (size == null || size <= 0) {
            throw new CloudRuntimeException("Invalid volume size provided: " + size);

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot AI review requested due to automatic review settings August 10, 2026 05:09

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (3)

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:564

  • Potential NullPointerException: iface.getIp() is dereferenced without a null check (iface.getIp().getAddress()). If ONTAP returns an interface record without an IP object, this will NPE and abort LIF selection. Consider skipping records with missing IP/address before calling isIPv4Address(...).
            for (IpInterface iface : response.getRecords()) {
                if (!Boolean.TRUE.equals(iface.getEnabled()) || !OntapStorageConstants.LIF_STATE_UP.equals(iface.getState())) {
                    continue;
                }
                if (!isIPv4Address(iface.getIp().getAddress())) {
                    continue;

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java:165

  • processDataLifSelection(...) can send a storage alert when a LIF warning is present. With the new ordering, this alert may fire before FlexVol creation succeeds; if volume creation later fails, operators may see alerts for a pool that never got created. Consider splitting LIF validation (fail fast before volume creation) from alert emission (only after volume creation succeeds).
            Pair<String, String> lifResult;
            try {
                lifResult = storageStrategy.getNetworkInterface(aggregate);
            } catch (Exception e) {
                logger.error("Exception occurred while retrieving network interface for pool: " + storagePoolName, e);
                throw new CloudRuntimeException("Failed to retrieve Data LIF from ONTAP: " + e.getMessage(), e);
            }
            processDataLifSelection(lifResult, details, storagePoolName, zoneId, podId);

            logger.info("Creating ONTAP volume '" + storagePoolName + "' with size: " + capacityBytes + " bytes (" +

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:224

  • chooseAggregate() is documented and implemented to pick the online aggregate with the largest available block space, but connect(true) currently calls validateAndSelectAggregatesForVolumeCreation(...), which overwrites this.aggregates with List.of(aggr) on each match. That means chooseAggregate() will typically see only the last eligible aggregate rather than all candidates, so the “largest available” selection can be wrong depending on SVM aggregate ordering.
     * Selects the best aggregate for a volume of the given size from candidates populated by
     * {@link #connect(boolean)} with aggregate validation enabled.
     *
     * <p>Picks the online aggregate with the largest available block space that can fit
     * {@code size}. The returned aggregate includes node information for LIF affinity.</p>
     *
     * @param size requested volume size in bytes
     * @return the chosen aggregate detail response

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants